[SPARK-30688][SQL] Week based dates not being parsed with TimestampFormatter by javierivanov · Pull Request #27498 · apache/spark

javierivanov · 2020-02-08T05:51:57Z

What changes were proposed in this pull request?

When executing the following query using a week based pattern the result is null.

scala> spark.sql("select unix_timestamp('2020-10', 'YYYY-ww')").show
+--------------------------------+
|unix_timestamp(2020-10, YYYY-ww)|
+--------------------------------+
|                            null|
+--------------------------------+

You can replicate this behavior with:

import java.time.temporal._
import java.time.format._
import java.util.Locale
import java.time.chrono.IsoChronology

val pattern = "YYYY-ww"
val locale = Locale.US
val s = "2020-10"
val formatter = new DateTimeFormatterBuilder().appendPattern(pattern)
    .parseDefaulting(ChronoField.ERA, 1)
    .parseDefaulting(ChronoField.MONTH_OF_YEAR, 1)
    .parseDefaulting(ChronoField.DAY_OF_MONTH, 1)
    .parseDefaulting(ChronoField.MINUTE_OF_HOUR, 0)
    .parseDefaulting(ChronoField.SECOND_OF_MINUTE, 0)
    .toFormatter(locale)
    .withChronology(IsoChronology.INSTANCE)
    .withResolverStyle(ResolverStyle.STRICT)

val result = formatter.parse(s)
result.query(TemporalQueries.localDate)

The date is parsed, but not resolved.

This is caused by the default temporal fields that are conflicting with the week based values:

      .parseDefaulting(ChronoField.ERA, 1)
      .parseDefaulting(ChronoField.MONTH_OF_YEAR, 1)
      .parseDefaulting(ChronoField.DAY_OF_MONTH, 1)

To avoid this conflict, I propose to check if the pattern is a week/year based and change the defaults as follows:

      .parseDefaulting(ChronoField.ERA, 1)
      .parseDefaulting(ChronoField.DAY_OF_WEEK, 1)

Note:

Additonally in JDK8 there is know issue for patterns (YYYYww/YYww) https://bugs.openjdk.java.net/browse/JDK-8145633 which fails to parse contiguos patterns without separation, this has been fixed in version 9. To avoid parsing issues add a separator like this (YYYY-ww/YYYY ww)

Why are the changes needed?

Week/Year based dates should be supported since it is part of ISO8601. Also, as seen in SPARK-30688, this issue is failing silently.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Passing current tests
Added test case for different timezones in DateExpressionsSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala

HyukjinKwon · 2020-02-10T10:46:26Z

cc @MaxGekk since you're touching the codes around here.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala

AmplabJenkins · 2020-03-19T12:01:56Z

Can one of the admins verify this patch?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateFormatter.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala

javierivanov · 2020-05-06T14:59:28Z

ping @MaxGekk @HyukjinKwon :)

cloud-fan · 2020-05-19T06:23:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala

+    } else {
+      Seq(
+        (ChronoField.MONTH_OF_YEAR, 1),
+        (ChronoField.DAY_OF_MONTH, 1))


I found these default values are very fragile to be set here, so I remove them all in #28576 . Can you check if it fixes your problem?

github-actions · 2020-08-28T00:43:18Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

javierivanov added 3 commits February 7, 2020 22:39

added fix

3edc102

added UT and comments

7698a6d

lint fix

0bb8cef

HyukjinKwon reviewed Feb 10, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala Outdated Show resolved Hide resolved

MaxGekk reviewed Feb 10, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala Outdated Show resolved Hide resolved

dongjoon-hyun added the SQL label Feb 10, 2020

javierivanov added 6 commits February 10, 2020 16:31

added setDefaulting method

c473d78

lint fix

89175d3

Merge remote-tracking branch 'upstream/master' into SPARK-30688

28b62db

added fix Locale issues

b4d7bd9

Merge remote-tracking branch 'upstream/master' into SPARK-30688

2093fa3

lint fix

8c8e996

merged

2db3ac0

javierivanov commented Apr 13, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateFormatter.scala Outdated Show resolved Hide resolved

MaxGekk reviewed Apr 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateFormatter.scala Outdated Show resolved Hide resolved

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala Outdated Show resolved Hide resolved

fixes

3557bbb

cloud-fan reviewed May 19, 2020

View reviewed changes

github-actions bot added the Stale label Aug 28, 2020

github-actions bot closed this Aug 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30688][SQL] Week based dates not being parsed with TimestampFormatter#27498

[SPARK-30688][SQL] Week based dates not being parsed with TimestampFormatter#27498
javierivanov wants to merge 11 commits intoapache:masterfrom
javierivanov:SPARK-30688

javierivanov commented Feb 8, 2020 •

edited

Loading

Uh oh!

Uh oh!

HyukjinKwon commented Feb 10, 2020

Uh oh!

Uh oh!

AmplabJenkins commented Mar 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javierivanov commented May 6, 2020

Uh oh!

cloud-fan May 19, 2020

Uh oh!

github-actions bot commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

javierivanov commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Note:

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

HyukjinKwon commented Feb 10, 2020

Uh oh!

Uh oh!

AmplabJenkins commented Mar 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javierivanov commented May 6, 2020

Uh oh!

cloud-fan May 19, 2020

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

javierivanov commented Feb 8, 2020 •

edited

Loading